Closed
Bug 928808
Opened 11 years ago
Closed 10 years ago
Clang trunk: mozalloc error: replacement function 'operator new' cannot be declared 'inline' [-Winline-new-delete]
Categories
(Core :: General, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: octoploid, Assigned: glandium)
References
(Blocks 2 open bugs, )
Details
Attachments
(4 files, 2 obsolete files)
2.76 KB,
patch
|
Details | Diff | Splinter Review | |
6.62 KB,
patch
|
Details | Diff | Splinter Review | |
6.89 KB,
patch
|
Details | Diff | Splinter Review | |
1.51 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release) Build ID: 20131020165230 Steps to reproduce: Try to build Firefox with Clang trunk. Actual results: In file included from /home/markus/mozilla-central/memory/mozalloc/mozalloc.cpp:29: ../../dist/include/mozilla/mozalloc.h:198:21: error: replacement function 'operator new' cannot be declared 'inline' MOZALLOC_EXPORT_NEW MOZALLOC_INLINE ^ ../../dist/include/mozilla/mozalloc.h:44:27: note: expanded from macro 'MOZALLOC_INLINE' # define MOZALLOC_INLINE MOZ_ALWAYS_INLINE_EVEN_DEBUG ^ ../../dist/include/mozilla/Attributes.h:43:75: note: expanded from macro 'MOZ_ALWAYS_INLINE_EVEN_DEBUG' # define MOZ_ALWAYS_INLINE_EVEN_DEBUG __attribute__((always_inline)) MOZ_INLINE ^ ../../dist/include/mozilla/Attributes.h:21:33: note: expanded from macro 'MOZ_INLINE' # define MOZ_INLINE inline ^ In file included from /home/markus/mozilla-central/memory/mozalloc/mozalloc.cpp:29: ../../dist/include/mozilla/mozalloc.h:204:21: error: replacement function 'operator new' cannot be declared 'inline' MOZALLOC_EXPORT_NEW MOZALLOC_INLINE ^ ../../dist/include/mozilla/mozalloc.h:44:27: note: expanded from macro 'MOZALLOC_INLINE' # define MOZALLOC_INLINE MOZ_ALWAYS_INLINE_EVEN_DEBUG ^ ../../dist/include/mozilla/Attributes.h:43:75: note: expanded from macro 'MOZ_ALWAYS_INLINE_EVEN_DEBUG' # define MOZ_ALWAYS_INLINE_EVEN_DEBUG __attribute__((always_inline)) MOZ_INLINE ^ ../../dist/include/mozilla/Attributes.h:21:33: note: expanded from macro 'MOZ_INLINE' # define MOZ_INLINE inline ^ In file included from /home/markus/mozilla-central/memory/mozalloc/mozalloc.cpp:29: ... Expected results: See: http://cplusplus.github.io/LWG/lwg-active.html#2340
Moving the definitions from mozalloc.h to memory/mozalloc/mozalloc.cpp (just leaving the declarations) and un-inline them all, seems to fix the issue. Does this look reasonable?
Assignee | ||
Comment 2•11 years ago
|
||
(In reply to Octoploid from comment #1) > Does this look reasonable? Not at all.
(In reply to Mike Hommey [:glandium] from comment #2) > (In reply to Octoploid from comment #1) > > Does this look reasonable? > > Not at all. You don't say so.
Comment 4•11 years ago
|
||
This is the clang commit which added this check: <https://github.com/llvm-mirror/clang/commit/d2b0cf38> It seems like they're right indeed, and our code is not C++11.
17.6.4.6 Replacement functions [replacement.functions] Clauses 18 through 30 and Annex D describe the behavior of numerous functions defined by the C ++ standard library. Under some circumstances, however, certain of these function descriptions also apply to replacement functions defined in the program (17.3). A C ++ program may provide the definition for any of eight dynamic memory allocation function signatures declared in header <new> (3.7.4, 18.6): — operator new(std::size_t) — operator new(std::size_t, const std::nothrow_t&) — operator new[](std::size_t) — operator new[](std::size_t, const std::nothrow_t&) — operator delete(void*) — operator delete(void*, const std::nothrow_t&) — operator delete[](void*) — operator delete[](void*, const std::nothrow_t&) The program’s definitions are used instead of the default versions supplied by the implementation (18.6). Such replacement occurs prior to program startup (3.2, 3.6). The program’s definitions shall not be specified as inline. No diagnostic is required.
Comment 6•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #6) > Created attachment 820495 [details] [diff] [review] > Patch (v1) Your patch won't work, because you'll get link errors later on.
Comment 8•11 years ago
|
||
Updated•11 years ago
|
Assignee: nobody → ekr
Comment 9•11 years ago
|
||
The patch in c8 compiles and links. I do not claim that it is ideal, but it should get people unwedged.
Comment 10•11 years ago
|
||
De-assigning this from me, since glandium explicitly says my approach in c8 is the wrong thing. Sounds like we need someone to figure out what an acceptable fix is and that's probably not me.
Assignee: ekr → nobody
Comment 11•11 years ago
|
||
(In reply to Eric Rescorla (:ekr) from comment #10) > De-assigning this from me, since glandium explicitly says my approach in c8 > is the wrong thing. Sounds like we need someone to figure out what an > acceptable fix is and that's probably not me. That would be glandium. :-)
Flags: needinfo?(mh+mozilla)
Comment 12•11 years ago
|
||
Added
Updated•11 years ago
|
Assignee: nobody → ekr
Updated•11 years ago
|
Attachment #820780 -
Attachment is obsolete: true
Updated•11 years ago
|
Assignee: ekr → nobody
Updated•11 years ago
|
Blocks: asan-maintenance
Assignee | ||
Comment 13•11 years ago
|
||
I'm out of short term workarounds. I'm afraid we'll have to go the hard way, which is to merge mozalloc in mozglue. That was on my todo list, and i have a half-year old, half working patch. IIRC the main issue was with windows. If that's really the case, maybe i can come up with something that keeps it on windows for now until those subtle details are figured.
Assignee: nobody → mh+mozilla
Flags: needinfo?(mh+mozilla)
Comment 14•11 years ago
|
||
This bit me with clang 3.4 from mac ports this week. :(
Comment 15•11 years ago
|
||
Fwiw, David Majnemer told me in the #llvm IRC channel that this has nothing to do with C++11: <majneme__> Mozilla shouldn't have those things inline, nasty things can happen <majneme__> It is an extension of the following defect report: http://cplusplus.github.io/LWG/lwg-defects.html#404 <majneme__> That LWG DR applies to C++03 too At least Clang doesn't support any standard where this would be valid to do. Not sure if it affects the way we want to fix this.
Updated•11 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 16•11 years ago
|
||
"Fixed" in Clang. See http://llvm.org/bugs/show_bug.cgi?id=17591 for details.
Comment 17•11 years ago
|
||
(In reply to comment #16) > "Fixed" in Clang. See http://llvm.org/bugs/show_bug.cgi?id=17591 for details. So should we mark these functions as __attribute__((used))?
Reporter | ||
Comment 18•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #17) > (In reply to comment #16) > > "Fixed" in Clang. See http://llvm.org/bugs/show_bug.cgi?id=17591 for details. > > So should we mark these functions as __attribute__((used))? I think this blows up the object size unnecessarily. E.g. peak memory use of the final libxul link during a clang LTO build went from 4.8 GB to 5.3 GB. So maybe simply adding -Wno-inline-new-delete to CXXFLAGS would be better, because the address of operator new/delete is taken nowhere AFAICS.
Comment 19•11 years ago
|
||
What's the status on this? It would be nice if we could use Clang tip again, especially for the Sanitizers. TSan for example is suffering some severe issues in older versions.
Blocks: tsan
Comment 20•11 years ago
|
||
(In reply to comment #18) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #17) > > (In reply to comment #16) > > > "Fixed" in Clang. See http://llvm.org/bugs/show_bug.cgi?id=17591 for details. > > > > So should we mark these functions as __attribute__((used))? > > I think this blows up the object size unnecessarily. E.g. peak memory use > of the final libxul link during a clang LTO build went from 4.8 GB to 5.3 GB. > So maybe simply adding -Wno-inline-new-delete to CXXFLAGS would be better, > because the address of operator new/delete is taken nowhere AFAICS. That sounds good to me.
Reporter | ||
Comment 21•11 years ago
|
||
I don't know the build machinery good enough, but checking for (__clang_major__ * 10 + __clang_minor__ >= 34) and adding -Wno-inline-new-delete in this case should be enough. (In reply to Christian Holler (:decoder) from comment #19) > What's the status on this? It would be nice if we could use Clang tip again, > especially for the Sanitizers. TSan for example is suffering some severe > issues in older versions. You could use Clang tip without problems. You will just get many warnings if -Wno-inline-new-delete isn't set.
Comment 22•11 years ago
|
||
Comment 23•11 years ago
|
||
I just hit this with Clang 3.4, with --enable-warnings-as-errors in my mozconfig. Should I stop using -enable-warnings-as-errors, or avoid using clang 3.4 for now?
Comment 24•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #20) > > So maybe simply adding -Wno-inline-new-delete to CXXFLAGS would be better, > > because the address of operator new/delete is taken nowhere AFAICS. > > That sounds good to me. Here's a hackaround patch that does this, FWIW. (not necessarily posting as an actual fix; more as a local workaround for folks to use if they like) Someone who understands the issues here better than I do: feel free to take this, improve the comment to make it meaningful, and request review on it. (or alternately, suggest some explanatory text in a bug-comment here, and I can incorporate it into the patch & request review myself)
Updated•11 years ago
|
Attachment #8365643 -
Attachment description: hackaround → hackaround: turn off Winline-new-delete in configure.in, for compilers that recognize it
Updated•11 years ago
|
Summary: Clang trunk: mozalloc error: replacement function 'operator new' cannot be declared 'inline' → Clang trunk: mozalloc error: replacement function 'operator new' cannot be declared 'inline' [-Winline-new-delete]
Reporter | ||
Comment 25•10 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #24) > Created attachment 8365643 [details] [diff] [review] > hackaround: turn off Winline-new-delete in configure.in, for compilers that > recognize it > > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #20) > > > So maybe simply adding -Wno-inline-new-delete to CXXFLAGS would be better, > > > because the address of operator new/delete is taken nowhere AFAICS. > > > > That sounds good to me. > > Here's a hackaround patch that does this, FWIW. (not necessarily posting as > an actual fix; more as a local workaround for folks to use if they like) > > Someone who understands the issues here better than I do: feel free to take > this, improve the comment to make it meaningful, and request review on it. > (or alternately, suggest some explanatory text in a bug-comment here, and I > can incorporate it into the patch & request review myself) > we inline 'new' and 'delete' in mozalloc (XXXwhy?) The only reason is performance. And because both gcc and clang accept inline replacement operators new+delete there is no reason not to inline. BTW, because the standard library must accept replacement new+delete, some people even provide definitions in their code that is roughly equivalent to the standard one, just so it could be inlined. There are discussions to add a -D_GLIBCXX_FORCE_DEFAULT_ALLOCATION to libstdc++ in gcc, that would only provide the inline versions. (Obviously replacement operators new+delete wouldn't work in this case.)
Comment 26•10 years ago
|
||
Mike, how should we proceed here? It seems like there are several ideas floating around on how to resolve this issue, and it would be nice if we took some solution here. Thanks!
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 27•10 years ago
|
||
AIUI, there's nothing to do here, except maybe hiding the warning, since clang doesn't error out anymore.
Flags: needinfo?(mh+mozilla)
Comment 28•10 years ago
|
||
(In reply to comment #27) > AIUI, there's nothing to do here, except maybe hiding the warning, since clang > doesn't error out anymore. It will if you treat warnings as errors! So is attachment 8365643 [details] [diff] [review] the right fix?
Comment 29•10 years ago
|
||
Perhaps -Wno-error=inline-new-delete?
Comment 30•10 years ago
|
||
That's effectively what attachment 8365643 [details] [diff] [review] does, behind some autoconf magic. (though -Wno instead of -Wno-error, because there's no point in even spamming a warning about this if we decide it's not fix-worthy)
Assignee | ||
Comment 31•10 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #28) > So is attachment 8365643 [details] [diff] [review] the right fix? Yes
Comment 32•10 years ago
|
||
OK; I'll plan on tweaking the code-comment in that patch to be more useful/informative, & I'll repost it for review soon.
Flags: needinfo?(dholbert)
Comment 33•10 years ago
|
||
OK, here's the same hackaround patch, but now with a more useful code-comment.
Attachment #8365643 -
Attachment is obsolete: true
Attachment #8374347 -
Flags: review?(mh+mozilla)
Flags: needinfo?(dholbert)
Assignee | ||
Updated•10 years ago
|
Attachment #8374347 -
Flags: review?(mh+mozilla) → review+
Comment 34•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc69628e2ad1
Flags: in-testsuite-
Comment 35•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bc69628e2ad1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment 36•9 years ago
|
||
This happens again on my mbp...
Comment 37•9 years ago
|
||
Yup, see bug 1155393.
Comment 38•8 years ago
|
||
We are now hitting the equivalent warning with VisualStudio 2015 Pro Update 2: 1:53.47 In the directory /c/Users/cpearce/src/central/objdir/dom/cellbroadcast 1:53.47 The following command failed to execute properly: 1:53.47 c:/Users/cpearce/src/central/objdir/_virtualenv/Scripts/python.exe -m mozbuild.action.cl cl -FoUnified_cpp_dom_cellbroadcast0.obj -c -Ic:/Users/cpearce/src/central/objdir/dist/stl_wrappers -DDEBUG=1 -DTRACING=1 -DWIN32_LEAN_AND_MEAN -D_WIN32 -DWIN32 -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DOS_WIN=1 -D_UNICODE -DCHROMIUM_BUILD -DU_STATIC_IMPLEMENTATION -DUNICODE -D_WINDOWS -D_SECURE_ATL -DCOMPILER_MSVC -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -Ic:/Users/cpearce/src/central/dom/cellbroadcast -Ic:/Users/cpearce/src/central/objdir/dom/cellbroadcast -Ic:/Users/cpearce/src/central/objdir/ipc/ipdl/_ipdlheaders -Ic:/Users/cpearce/src/central/ipc/chromium/src -Ic:/Users/cpearce/src/central/ipc/glue -Ic:/Users/cpearce/src/central/objdir/dist/include -Ic:/Users/cpearce/src/central/objdir/dist/include/nspr -Ic:/Users/cpearce/src/central/objdir/dist/include/nss -MD -FI c:/Users/cpearce/src/central/objdir/mozilla-config.h -DMOZILLA_CLIENT -TP -nologo -wd5026 -wd5027 -Zc:sizedDealloc- -Zc:threadSafeInit- -wd4091 -D_HAS_EXCEPTIONS=0 -W3 -Gy -arch:IA32 -FS -wd4251 -wd4244 -wd4267 -wd4345 -wd4351 -wd4800 -wd4819 -we4553 -GR- -Zi -O1 -Oi -Oy- -WX -Fdgenerated.pdb c:/Users/cpearce/src/central/objdir/dom/cellbroadcast/Unified_cpp_dom_cellbroadcast0.cpp 1:53.47 c:/Users/cpearce/src/central/config/rules.mk:918: recipe for target 'Unified_cpp_dom_cellbroadcast0.obj' failed 1:53.47 mozmake.EXE[5]: *** [Unified_cpp_dom_cellbroadcast0.obj] Error 1 1:53.47 c:/Users/cpearce/src/central/config/recurse.mk:71: recipe for target 'dom/cellbroadcast/target' failed 1:53.47 mozmake.EXE[4]: *** [dom/cellbroadcast/target] Error 2 1:56.27 Unified_cpp_dom_presentation0.cpp 1:56.27 c:\users\cpearce\src\central\objdir\dist\include\mozilla/mozalloc.h(184): error C2220: warning treated as error - no 'object' file generated 1:56.27 Warning: C4595 in c:\users\cpearce\src\central\objdir\dist\include\mozilla\mozalloc.h: 'operator new': non-member operator new or delete functions may not be declared inline 1:56.27 c:\users\cpearce\src\central\objdir\dist\include\mozilla/mozalloc.h(184): warning C4595: 'operator new': non-member operator new or delete functions may not be declared inline 1:56.27 c:\users\cpearce\src\central\objdir\dist\include\mozilla/mozalloc.h(184): note: to simplify migration, consider the temporary use of /Wv:18 flag with the version of the compiler with which you used to build without warnings
Comment 39•8 years ago
|
||
I filed bug 1261719 for the Visual Studio 2015 Update 2 case.
Comment 40•8 years ago
|
||
Firefox rv:48.0 "Patch: turn off Winline-new-delete in configure.in, for compilers that recognize it" is not present in old-configure.in. "MOZ_CXX_SUPPORTS_WARNING(-Wno-, inline-new-delete, ac_cxx_has_wno_inline_new_delete)" ---- begin cut ---- In file included from /var/spool/makepkg/firefox/src/firefox-48.0/toolkit/library/StaticXULComponentsStart.cpp:1: In file included from /var/spool/makepkg/firefox/src/firefox-48.0/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Module.h:10: In file included from /var/spool/makepkg/firefox/src/firefox-48.0/obj-x86_64-pc-linux-gnu/dist/include/nscore.h:20: /var/spool/makepkg/firefox/src/firefox-48.0/obj-x86_64-pc-linux-gnu/dist/include/mozilla/mozalloc.h:225:21: warning: replacement function 'operator delete[]' cannot be declared 'inline' [-Winline-new-delete] MOZALLOC_EXPORT_NEW MOZALLOC_INLINE ^ /var/spool/makepkg/firefox/src/firefox-48.0/obj-x86_64-pc-linux-gnu/dist/include/mozilla/mozalloc.h:31:27: note: expanded from macro 'MOZALLOC_INLINE' # define MOZALLOC_INLINE MOZ_ALWAYS_INLINE_EVEN_DEBUG ^ /var/spool/makepkg/firefox/src/firefox-48.0/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Attributes.h:27:75: note: expanded from macro 'MOZ_ALWAYS_INLINE_EVEN_DEBUG' # define MOZ_ALWAYS_INLINE_EVEN_DEBUG __attribute__((always_inline)) inline ^ 8 warnings generated. libxul_s.a.desc libxul.so ---- end cut ----
You need to log in
before you can comment on or make changes to this bug.
Description
•